Add `SecWebsocketExtensions` #88

kazk · 2021-09-13T20:50:58Z

I'm working on seanmonstar/warp#770 and noticed this is missing.

seanmonstar

Thanks for the PR!

Does this type by itself provide any use? I imagine you need to do more with it than simply materialize an opaque value from the map.

seanmonstar

Wow, this is a lot of work, thanks for putting so much thought into this!

seanmonstar · 2021-09-17T17:34:07Z

src/common/sec_websocket_extensions.rs

+/// [RFC6455_11.3.2]: https://tools.ietf.org/html/rfc6455#section-11.3.2
+/// [RFC7692_7]: https://tools.ietf.org/html/rfc7692#section-7
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct SecWebsocketExtensions(pub Vec<WebsocketExtension>);


We should keep the fields private, so we can change the internal implementation whenever desired.

I explained the reason behind this in #88 (comment)

seanmonstar · 2021-09-17T17:41:47Z

src/common/sec_websocket_extensions.rs

+
+/// A WebSocket extension containing the name and parameters.
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct WebsocketExtension {


This just a thought, let me know what you think. What if this type simply held a reference, instead of individual owned value strings and a vector of params? The name and params methods could return "lazy" values, as a few of the other headers in this crate already do.

I actually had struct SecWebsocketExtensions(HeaderValueString); with lazy iter() originally. But I wanted decode to fail on values with invalid syntax, so typed_try_get can be used to detect them. This requires parsing the values, so I thought it was better to store the result.

The main reason this can be useful is, when negotiating extensions, values with invalid syntax must be rejected:

If a value is received by either the client or the server during negotiation that does not conform to the ABNF below, the recipient of such malformed data MUST immediately Fail the WebSocket Connection.

https://datatracker.ietf.org/doc/html/rfc6455#section-9.1

If we validate the syntax in decode, crates using headers can focus on the parameters for each extension, and warp can reject the invalid headers, etc..

Also, having WebsocketExtension independent is useful because the server needs to filter them, and respond with potentially mutated versions (e.g., parameter ("client_max_window_bits", Some("10")) may be changed to have value Some("9") in the response). WebsocketExtension can provide setters/builder that makes sure that the value won't result in invalid syntax.

This is why I made the field and the struct public, but I can try to come up with something else if this is an issue.

seanmonstar · 2021-09-17T17:42:35Z

src/common/sec_websocket_extensions.rs

+
+    /// An iterator over the `WebsocketExtension`s in `SecWebsocketExtensions` header(s).
+    pub fn iter(&self) -> impl Iterator<Item = &WebsocketExtension> {
+        self.0.iter()


Similar question here, what if instead of owning a Vec, the return iterator were lazy, and did the parsing as requested on each call to next, like other headers?

ilammy · 2022-10-17T09:36:53Z

I'd like to take over pushing permessage-deflate through the WebSockets stack.

Which review points have to be addressed in order to get this PR merged?

kazk · 2022-10-17T22:09:58Z

@ilammy I was waiting on a response from @seanmonstar for #88 (comment).

Once this is merged, I already have a branch using it, which is a rebased version of my original tungstenite-rs PR.

gallowsmaker · 2023-01-05T02:49:17Z

@kazk @seanmonstar I'm humble ask to return to review this PR as dependent functionality in tungstenite-rs extremely important for my team, but in same time we have strict policy don't use libraries from branches, so we have to wait for finalized published version.

kazk · 2023-01-06T04:21:08Z

@gallowsmaker I can't do anything for this PR, but I'll open a new PR for tungstenite-rs so we can start reviewing the new version while we wait for this. It will also clarify what needs to be done. Looks like I need to rebase again and there are some conflicts, but I should be able to open it soon.

I've been using the original version in production without any issues, but I'd love to get these merged and move on.

nicknotfun · 2023-02-03T19:37:30Z

@seanmonstar just checking if you're still able to review this?

nakedible-p · 2023-04-12T22:50:03Z

It would be super nice to get this merged - any hope for this to progress?

ilammy · 2023-11-13T12:14:31Z

@seanmonstar @kazk bump. What's the blocker here? What help do you need?

I need use SecWebsocketExtensions hyperium#88 , but it not merge, pub util so I can copy the file to my project without merge it into headers , the file need util is public

kaszperro · 2023-12-18T15:51:45Z

This PR is highly needed feature. It is blocker for us for using permessage-deflate. What can I do, to speedup merging of this?

ddimaria · 2024-02-01T18:14:44Z

@seanmonstar are there any plans for this to be merged, or do you require any changes to be made? Getting this merged unblocks a PR in tungstenite that provides permessage-deflate support for websockets.

AdrianEddy · 2024-04-02T01:59:01Z

@seanmonstar What would it take to get this merged? I'm willing to sponsor this work

seanmonstar · 2024-04-02T20:10:01Z

I know there's a lot of interest here. I've describe this a few times in chat, but never here. So, let me try:

This crate tries to expose built-in header types which expose as little internal details as possible. The reason for that is to try to limit mistakes that occurred when this was hyper::header in older versions, and changes to the exposed internals would mean breaking changes.
It's not clear whether SecWebsocketExtensions should be optimized for parsing, or for mutating.
- That changes whether it should internally hold onto a flattened string, and parse on the fly of fn iter() -> impl Iterator, or if it should store everything parsed at construction in a list.
- Unfortunately, that internal decision seems like it will leak, since the iterator would either provide WebsocketExtension<'a>s referring to the flat string, or &WebsocketExtension if an owned item inside a list.
- Most asking for this feature are more interested in it just working (understandably), than the maintenance details afterwards.
This doesn't need to block people. This is the most important thing that is missed. Since the crate is based on a trait Header, any other code is able to declare its own typed header and impl Header for MySecWebSocketExtensions.
- That code can start in tungstenite, or wherever else that's wanting this, and the design and performance of the type can be tested and tweaked there.
- Once those questions are answered and tested, we can merge something here for longer term maintenance.

seanmonstar reviewed Sep 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

kazk force-pushed the sec-websocket-extensions branch 3 times, most recently from b60896b to ff67b0b Compare September 17, 2021 06:37

kazk marked this pull request as draft September 17, 2021 06:51

kazk force-pushed the sec-websocket-extensions branch 5 times, most recently from c6ee5f0 to 16e387a Compare September 17, 2021 07:55

kazk marked this pull request as ready for review September 17, 2021 07:59

kazk requested a review from seanmonstar September 17, 2021 08:00

seanmonstar reviewed Sep 17, 2021

View reviewed changes

kazk force-pushed the sec-websocket-extensions branch 2 times, most recently from 50ae3d2 to f6fb712 Compare September 17, 2021 23:10

kazk mentioned this pull request Nov 3, 2021

Add permessage-deflate support snapview/tungstenite-rs#235

Closed

5 tasks

kazk marked this pull request as draft March 14, 2022 06:07

Add SecWebsocketExtensions

a7c0056

kazk force-pushed the sec-websocket-extensions branch from f6fb712 to a7c0056 Compare March 14, 2022 06:08

kazk mentioned this pull request Dec 2, 2022

Support for permessage-deflate [✅/awaiting_dependency_merge] snapview/tungstenite-rs#2

Open

kazk force-pushed the sec-websocket-extensions branch from 2177abb to a7c0056 Compare January 6, 2023 22:53

Make the field private

b9cfe47

kazk force-pushed the sec-websocket-extensions branch from 54fd7a1 to b9cfe47 Compare January 6, 2023 23:48

This was referenced Jan 7, 2023

Add permessage-deflate support snapview/tungstenite-rs#328

Merged

Add WebSocket permessage-deflate extension support seanmonstar/warp#1016

Draft

coolreader18 mentioned this pull request May 16, 2023

Adding SecWebsocketProtocol/SecWebsocketExtensions #138

Open

This comment was marked as spam.

Sign in to view

xxaier added a commit to xxaier/headers that referenced this pull request Nov 22, 2023

pub mod util

a929079

I need use SecWebsocketExtensions hyperium#88 , but it not merge, pub util so I can copy the file to my project without merge it into headers , the file need util is public

xxaier mentioned this pull request Nov 22, 2023

pub mod util #154

Closed

SvizelPritula mentioned this pull request May 16, 2024

Add permessage-deflate support, again snapview/tungstenite-rs#426

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SecWebsocketExtensions` #88

Add `SecWebsocketExtensions` #88

kazk commented Sep 13, 2021

seanmonstar left a comment

This comment has been minimized.

seanmonstar left a comment

seanmonstar Sep 17, 2021

kazk Sep 17, 2021

seanmonstar Sep 17, 2021

kazk Sep 17, 2021

seanmonstar Sep 17, 2021

ilammy commented Oct 17, 2022

kazk commented Oct 17, 2022

gallowsmaker commented Jan 5, 2023

kazk commented Jan 6, 2023

nicknotfun commented Feb 3, 2023

nakedible-p commented Apr 12, 2023

This comment was marked as spam.

ilammy commented Nov 13, 2023

This comment was marked as spam.

This comment was marked as spam.

kaszperro commented Dec 18, 2023

ddimaria commented Feb 1, 2024

AdrianEddy commented Apr 2, 2024

seanmonstar commented Apr 2, 2024

Add SecWebsocketExtensions #88

Are you sure you want to change the base?

Add SecWebsocketExtensions #88

Conversation

kazk commented Sep 13, 2021

seanmonstar left a comment

Choose a reason for hiding this comment

This comment has been minimized.

seanmonstar left a comment

Choose a reason for hiding this comment

seanmonstar Sep 17, 2021

Choose a reason for hiding this comment

kazk Sep 17, 2021

Choose a reason for hiding this comment

seanmonstar Sep 17, 2021

Choose a reason for hiding this comment

kazk Sep 17, 2021

Choose a reason for hiding this comment

seanmonstar Sep 17, 2021

Choose a reason for hiding this comment

ilammy commented Oct 17, 2022

kazk commented Oct 17, 2022

gallowsmaker commented Jan 5, 2023

kazk commented Jan 6, 2023

nicknotfun commented Feb 3, 2023

nakedible-p commented Apr 12, 2023

This comment was marked as spam.

ilammy commented Nov 13, 2023

This comment was marked as spam.

This comment was marked as spam.

kaszperro commented Dec 18, 2023

ddimaria commented Feb 1, 2024

AdrianEddy commented Apr 2, 2024

seanmonstar commented Apr 2, 2024

Add `SecWebsocketExtensions` #88

Add `SecWebsocketExtensions` #88